-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Several performance optimizations #100
base: develop
Are you sure you want to change the base?
Conversation
…nload throughput. Most of this is by switching to release build, but other optimizations got a combined ~1.5x increase too, as tested on m6i.8xlarge instances with EBS VPC Endpoint.
…ocks; more thread tweaking.
… m6i.16xlarge instance with EBS VPC Endpoint.
Thanks for this, @kdavyd! We're giving it a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
const SHA256_ALGORITHM: &str = "SHA256"; | ||
const DISABLE_SHA: bool = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't want to turn off the SHA hash checking by default. I could maybe see providing users a flag to turn this off, but a constant isn't really configurable, and isn't currently documented or particularly discoverable.
@@ -355,7 +358,9 @@ impl SnapshotDownloader { | |||
.await | |||
.context(error::WriteFileBytes { path, count })?; | |||
|
|||
f.flush().await.context(error::FlushFile { path })?; | |||
if !DISABLE_SHA { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting looks a bit off here - can we make sure to run all the code through rustfmt
?
type Result<T> = std::result::Result<T, error::Error>; | ||
|
||
#[tokio::main] | ||
#[tokio::main(flavor = "multi_thread", worker_threads = 512)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious how you calculated or determined this number!
rusoto-native-tls = ["rusoto_core/native-tls", "rusoto_ebs/native-tls", "rusoto_ec2/native-tls"] | ||
rusoto-rustls = ["rusoto_core/rustls", "rusoto_ebs/rustls", "rusoto_ec2/rustls"] | ||
|
||
[dependencies] | ||
argh = "0.1.3" | ||
tokio = { version = "~1.8", features = ["fs", "io-util", "time"] } # LTS | ||
tokio = { version = "~1.11", features = ["fs", "io-util", "time"] } # LTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we're hoping to stay on tokio 1.8 as it's an LTS release.
About 4.5x overall increase in download throughput. Most of this is by switching to release build, but other optimizations got a combined ~1.5x increase too, as tested on m6i.8xlarge instances with EBS VPC Endpoint.
Focusing on download performance only. Best performance I got is around 230MB/sec on a fully allocated 100GB snapshot.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.